-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
report(tsc): infer createElement type from tag name #6637
Conversation
@@ -364,8 +364,7 @@ class ReportUIFeatures { | |||
// load event, however it is cross-domain and won't fire. Instead, listen | |||
// for a message from the target app saying "I'm open". | |||
const json = reportJson; | |||
window.addEventListener('message', function msgHandler(/** @type {Event} */ e) { | |||
const messageEvent = /** @type {MessageEvent} */ (e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea what was going on here. Maybe fixed on the compiler/built-in defs side so it was not only inferred, but inferred as a MessageEvent
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always down for fewer type assertions!
@@ -18,6 +18,8 @@ | |||
|
|||
/* globals URL self */ | |||
|
|||
/** @typedef {HTMLElementTagNameMap & {[id: string]: HTMLElement}} HTMLElmentByTagName */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTMLElementTagNameMap
is the built-in you're talking about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTMLElementTagNameMap
is the built-in you're talking about?
yes
/** | ||
* @param {string} name | ||
* @template {string} T | ||
* @param {T} name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we type this as the keys of the HTMLElementTagNameMap
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we type this as the keys of the
HTMLElementTagNameMap
?
We could, though it would eliminate the fallback to the generic HTMLElement
when a tag name isn't recognized. Maybe we're ok with that in exchange for better completion/stricter checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would we still get helpful completion if it were strict options | string? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strict options | string
sadly no, it's smart about it and coalesces to just string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I tried to use just HTMLElementTagNameMap
, and we have one place that's unrecognized: 'summary'
in
const summaryEl = this.dom.createChildOf(groupEl, expandable ? 'summary' : 'div'); |
When Edge gets <details>
support I imagine that they'll also support <summary>
and their WebIDL will get updated and so the tsc types will get updated (I'm surprised it already has 'details'
support, honestly), but until then we probably just want to go with how this PR is now.
This is a little silly, but there's a small quality of life improvement, it closes an old TODO, and it eliminates a decent number of type assertions.
Bases the created element type on the tag name passed in, so e.g.
DOM.createElement('a')
returns anHTMLAnchorElement
, not just a genericElement
. If the tag is unknown, falls back to justHTMLElement
, which is close enough to correct for us today.Uses the same tag-name-to-element-type map that tsc uses for
document.createElement
, just with a union for theHTMLElement
fallback instead of the function overload they use (and sinceDOM.createElement
callsdocument.createElement
internally, this type is checked by their type).